[clang][CodeGen] Fix sub-optimal clang CodeGen for __atomic_test_and_set #160098
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Sirui Mu (Lancern) ChangesClang CodeGen for bool f(void *ptr) {
return __atomic_test_and_set(ptr, __ATOMIC_RELAXED);
}%1 = atomicrmw xchg ptr %0, i8 1 monotonic, align 1
%tobool = icmp ne i8 %1, 0
store i1 %tobool, ptr %atomic-temp, align 1which could lead to suboptimal binary code, for example on x86_64: f:
mov al, 1
xchg byte ptr [rdi], al
test al, al
setne al
setne byte ptr [rsp - 1]
retThe last I'm quite conservative about the codegen in this patch. Vanilla gcc actually emits simpler code for bool f(void *ptr) {
return __atomic_test_and_set(ptr, __ATOMIC_RELAXED);
}f:
mov eax, 1
xchg al, BYTE PTR [rdi]
retIt seems like gcc assumes Related to #121943 . Full diff: https://github.com/llvm/llvm-project/pull/160098.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGAtomic.cpp b/clang/lib/CodeGen/CGAtomic.cpp
index 9106c4cd8e139..d44f8e1ea40eb 100644
--- a/clang/lib/CodeGen/CGAtomic.cpp
+++ b/clang/lib/CodeGen/CGAtomic.cpp
@@ -735,8 +735,10 @@ static void EmitAtomicOp(CodeGenFunction &CGF, AtomicExpr *E, Address Dest,
CGF.Builder.getInt8(1), Order, Scope, E);
RMWI->setVolatile(E->isVolatile());
llvm::Value *Result = CGF.Builder.CreateIsNotNull(RMWI, "tobool");
- auto *I = CGF.Builder.CreateStore(Result, Dest);
- CGF.addInstToCurrentSourceAtom(I, Result);
+ llvm::Value *ExtResult =
+ CGF.Builder.CreateZExt(Result, CGF.Builder.getInt8Ty(), "tobool.zext");
+ auto *I = CGF.Builder.CreateStore(ExtResult, Dest);
+ CGF.addInstToCurrentSourceAtom(I, ExtResult);
return;
}
diff --git a/clang/test/CodeGen/atomic-test-and-set.c b/clang/test/CodeGen/atomic-test-and-set.c
index 39d4cef16b21d..6438094567f33 100644
--- a/clang/test/CodeGen/atomic-test-and-set.c
+++ b/clang/test/CodeGen/atomic-test-and-set.c
@@ -81,7 +81,8 @@ void clear_dynamic(char *ptr, int order) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 monotonic, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -99,7 +100,8 @@ void test_and_set_relaxed(char *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 acquire, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -117,7 +119,8 @@ void test_and_set_consume(char *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 acquire, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -135,7 +138,8 @@ void test_and_set_acquire(char *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 release, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -153,7 +157,8 @@ void test_and_set_release(char *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 acq_rel, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -171,7 +176,8 @@ void test_and_set_acq_rel(char *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 seq_cst, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -200,27 +206,32 @@ void test_and_set_seq_cst(char *ptr) {
// CHECK: [[MONOTONIC]]:
// CHECK-NEXT: [[TMP2:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 monotonic, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP2]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: br label %[[ATOMIC_CONTINUE:.*]]
// CHECK: [[ACQUIRE]]:
// CHECK-NEXT: [[TMP3:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 acquire, align 1
// CHECK-NEXT: [[TOBOOL1:%.*]] = icmp ne i8 [[TMP3]], 0
-// CHECK-NEXT: store i1 [[TOBOOL1]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT1:%.*]] = zext i1 [[TOBOOL1]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT1]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: br label %[[ATOMIC_CONTINUE]]
// CHECK: [[RELEASE]]:
// CHECK-NEXT: [[TMP4:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 release, align 1
// CHECK-NEXT: [[TOBOOL2:%.*]] = icmp ne i8 [[TMP4]], 0
-// CHECK-NEXT: store i1 [[TOBOOL2]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT2:%.*]] = zext i1 [[TOBOOL2]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT2]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: br label %[[ATOMIC_CONTINUE]]
// CHECK: [[ACQREL]]:
// CHECK-NEXT: [[TMP5:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 acq_rel, align 1
// CHECK-NEXT: [[TOBOOL3:%.*]] = icmp ne i8 [[TMP5]], 0
-// CHECK-NEXT: store i1 [[TOBOOL3]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT3:%.*]] = zext i1 [[TOBOOL3]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT3]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: br label %[[ATOMIC_CONTINUE]]
// CHECK: [[SEQCST]]:
// CHECK-NEXT: [[TMP6:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 seq_cst, align 1
// CHECK-NEXT: [[TOBOOL4:%.*]] = icmp ne i8 [[TMP6]], 0
-// CHECK-NEXT: store i1 [[TOBOOL4]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT4:%.*]] = zext i1 [[TOBOOL4]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT4]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: br label %[[ATOMIC_CONTINUE]]
// CHECK: [[ATOMIC_CONTINUE]]:
// CHECK-NEXT: [[TMP7:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
@@ -239,7 +250,8 @@ void test_and_set_dynamic(char *ptr, int order) {
// CHECK-NEXT: [[ARRAYDECAY:%.*]] = getelementptr inbounds [10 x i32], ptr [[X]], i64 0, i64 0
// CHECK-NEXT: [[TMP0:%.*]] = atomicrmw volatile xchg ptr [[ARRAYDECAY]], i8 1 seq_cst, align 4
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP0]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP1:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP1]] to i1
// CHECK-NEXT: ret void
@@ -301,7 +313,8 @@ void clear_incomplete(struct incomplete *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 monotonic, align 4
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -318,7 +331,8 @@ void test_and_set_int(int *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 monotonic, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
@@ -335,7 +349,8 @@ void test_and_set_void(void *ptr) {
// CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[PTR_ADDR]], align 8
// CHECK-NEXT: [[TMP1:%.*]] = atomicrmw xchg ptr [[TMP0]], i8 1 monotonic, align 1
// CHECK-NEXT: [[TOBOOL:%.*]] = icmp ne i8 [[TMP1]], 0
-// CHECK-NEXT: store i1 [[TOBOOL]], ptr [[ATOMIC_TEMP]], align 1
+// CHECK-NEXT: [[TOBOOL_ZEXT:%.*]] = zext i1 [[TOBOOL]] to i8
+// CHECK-NEXT: store i8 [[TOBOOL_ZEXT]], ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[TMP2:%.*]] = load i8, ptr [[ATOMIC_TEMP]], align 1
// CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[TMP2]] to i1
// CHECK-NEXT: ret void
|
Lancern
force-pushed
the
clang/atomic-test-and-set-codegen
branch
from
504bfa6 to
bfa16a7
Compare
clang/lib/CodeGen/CGAtomic.cpp
Outdated
| @@ -735,8 +735,10 @@ static void EmitAtomicOp(CodeGenFunction &CGF, AtomicExpr *E, Address Dest, | |||
| CGF.Builder.getInt8(1), Order, Scope, E); | |||
| RMWI->setVolatile(E->isVolatile()); | |||
| llvm::Value *Result = CGF.Builder.CreateIsNotNull(RMWI, "tobool"); | |||
| auto *I = CGF.Builder.CreateStore(Result, Dest); | |||
| CGF.addInstToCurrentSourceAtom(I, Result); | |||
| llvm::Value *ExtResult = | |||
There was a problem hiding this comment.
For consistency with other code, this should use CodeGenFunction::EmitToMemory
Clang CodeGen for `__atomic_test_and_set` would emit a `store` instruction that
stores an `i1` value:
```cpp
bool f(bool *ptr) {
return __atomic_test_and_set(ptr, __ATOMIC_RELAXED);
}
```
```llvm
%1 = atomicrmw xchg ptr %0, i8 1 monotonic, align 1
%tobool = icmp ne i8 %1, 0
store i1 %tobool, ptr %atomic-temp, align 1
```
which could lead to suboptimal binary code, for example on x86_64:
```asm
f:
mov al, 1
xchg byte ptr [rdi], al
test al, al
setne al
setne byte ptr [rsp - 1]
ret
```
This patch fixes this issue by first extending the `i1` value to an `i8` before
the store. This effectively eliminates the last `setne` instruction in the
binary code sequence.
Lancern
force-pushed
the
clang/atomic-test-and-set-codegen
branch
from
bfa16a7 to
5ebd984
Compare
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
…set (llvm#160098) Clang CodeGen for `__atomic_test_and_set` would emit a `store` instruction that stores an `i1` value: ```cpp bool f(void *ptr) { return __atomic_test_and_set(ptr, __ATOMIC_RELAXED); } ``` ```llvm %1 = atomicrmw xchg ptr %0, i8 1 monotonic, align 1 %tobool = icmp ne i8 %1, 0 store i1 %tobool, ptr %atomic-temp, align 1 ``` which could lead to suboptimal binary code, for example on x86_64: ```asm f: mov al, 1 xchg byte ptr [rdi], al test al, al setne al setne byte ptr [rsp - 1] ret ``` The last `setne` instruction is obviously redundant. This patch fixes this issue by first zero-extending `%tobool` to an `i8` before the store. This effectively eliminates the last `setne` instruction in the binary code sequence. The `test` and `setne` on `al` is kept still, though. ----- I'm quite conservative about the codegen in this patch. Vanilla gcc actually emits simpler code for `__atomic_test_and_set`: ```cpp bool f(void *ptr) { return __atomic_test_and_set(ptr, __ATOMIC_RELAXED); } ``` ```asm f: mov eax, 1 xchg al, BYTE PTR [rdi] ret ``` It seems like gcc assumes `ptr` would always point to a valid `bool` value as required by the ABI. I'm not sure if we should also make this assumption. Related to llvm#121943 .
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clang CodeGen for
__atomic_test_and_setwould emit astoreinstruction that stores ani1value:which could lead to suboptimal binary code, for example on x86_64:
The last
setneinstruction is obviously redundant. This patch fixes this issue by first zero-extending%toboolto ani8before the store. This effectively eliminates the lastsetneinstruction in the binary code sequence. Thetestandsetneonalis kept still, though.I'm quite conservative about the codegen in this patch. Vanilla gcc actually emits simpler code for
__atomic_test_and_set:It seems like gcc assumes
ptrwould always point to a validboolvalue as required by the ABI. I'm not sure if we should also make this assumption.Related to #121943 .